Use pkg/fulcioroots and pkg/tuf from sigstore/sigstore #1866
Conversation
imjasonh
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1866 +/- ##
==========================================
- Coverage 33.43% 31.62% -1.81%
==========================================
Files 146 138 -8
Lines 9344 8632 -712
==========================================
- Hits 3124 2730 -394
+ Misses 5847 5581 -266
+ Partials 373 321 -52
Continue to review full report at Codecov.
|
cmd/cosign/cli/fulcio/fulcio.go
Outdated
| return intermediates | ||
| } | ||
|
|
||
| func initRoots() (*x509.CertPool, *x509.CertPool, error) { |
There was a problem hiding this comment.
Thanks for adding this here! I didn't realize this had already been added back.
|
I'm writing up a design doc right now, let's go as proceeded but I realize separating this from cosign means that it's very difficult to, for example, set locations of a remote through flags for cosign. My plan is to refactor the TUF client now in sigstore with a
The goal is all of these can be configured through cosign usage, either through env vars of flags. I'll share the doc today. I'm currently hacking on it because it's hard to pre-empt the design without trying it :/ |
|
Do we want to keep cosign's I think gitsign is using it, but I can update that myself. |
imjasonh
changed the title
I don't mind removing it. |
| // If the SIGSTORE_ROOT_FILE environment variable is set, the root config found | ||
| // there will be used instead of the normal Fulcio roots. | ||
| // | ||
| // If this behavior is not needed, use |
There was a problem hiding this comment.
nit: not clear whether "this behavior" refers to "returns the Fulcio root certificate" or the bit about the env var.
There was a problem hiding this comment.
+1, would simply clarify that these methods support fetching from an environment variable too
There was a problem hiding this comment.
I had wanted to have some wording around how/why this is different from s/s's pkg/fulcioroots, but I guess it doesn't really need it since this is internal now and only usable in cosign. I'll remove the "If this behavior" part. Thanks!
| singletonRootErr error | ||
| ) | ||
|
|
||
| const altRoot = "SIGSTORE_ROOT_FILE" |
There was a problem hiding this comment.
Move the documentation on If the SIGSTORE_ROOT_FILE env variable is set above to here?
Also, what is the format of this file -- notably it's a list of PEM encoded FULCIO certificates, NOT a TUF root.json.
There was a problem hiding this comment.
This const isn't exported, so the comment wouldn't be available in godoc. I can put a comment here too in case folks are looking through code and find it, but I think it's better to have on the exported method.
Can you suggest wording for the format of the file? I think you'll have better context there than me.
|
Merge conflict, otherwise lgtm and let's get this in to unblock the log jam. |
Signed-off-by: Jason Hall <[email protected]>
Signed-off-by: Jason Hall [email protected]
Depends on sigstore/sigstore#435
Part of #1865
Summary
This removes
pkg/cosign/tufin favor if sigstore/sigstore's newpkg/tuf.This moves cosign-specific logic into an
internalpackage only used by the cosign CLI. This package adds cosign-specific override behavior, and otherwise falls back to sigstore/sigstore's newpkg/fulcioroots.Release Note